Feature/kill button - adding the kill button on top of NodeTree #500

mikibonacci · 2023-10-09T10:20:15Z

Fixes #161

adding the kill button to support live-killing of a workchain, just selecting it from the list and clicking on the button.

unkcpz

Thanks @mikibonacci. Just two small comments.

src/aiidalab_qe/common/process.py

unkcpz · 2023-10-09T10:38:15Z

adding the kill button to support live-killing of a workchain, just selecting it from the list and clicking on the button.

I think for this button, it should be only enabled only when the workchain is in "running" state, so the state of the button should be update when the new process is selected from the workchain selector list.

codecov · 2023-10-09T10:39:00Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (74a1965) 78.60% compared to head (80632a5) 80.99%.

❗ Current head 80632a5 differs from pull request most recent head df1d435. Consider uploading reports for the commit df1d435 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
+ Coverage   78.60%   80.99%   +2.39%     
==========================================
  Files          44       45       +1     
  Lines        3160     3805     +645     
==========================================
+ Hits         2484     3082     +598     
- Misses        676      723      +47

Flag	Coverage Δ
python-3.10	`80.99% <64.28%> (+2.39%)`	⬆️
python-3.8	`81.03% <64.28%> (+2.38%)`	⬆️
python-3.9	`81.03% <64.28%> (+2.38%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/aiidalab_qe/common/process.py	`85.71% <ø> (ø)`
src/aiidalab_qe/app/result/__init__.py	`81.25% <64.28%> (-7.33%)`	⬇️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielhollas · 2023-10-09T12:09:51Z

Cool, I might steal this, wanted to have this feature for a while :-) Hower, my plan was to implement the button next somewhere next to the NodeTree widget of a running process. It seems a bit out of place in the Workchain selector, I would worry about accidentally clicking it. WDYT?

unkcpz · 2023-10-09T12:39:45Z

It seems a bit out of place in the Workchain selector, I would worry about accidentally clicking it. WDYT?

@danielhollas thanks a lot for chime in. It is a great point. My plan was to bring in this concept to the QeApp and we think about the layout afterward.
For the misclick, I won't worry about it too much, the window-close icon contains the cross, so I suppose the user will notice it will cause some fatal consequence (although we can not predict too much but how fatal it can be?). You idea of put it in the node tree is great, but I am afraid it is straightforward to implement especially when we have some issues concerned about improve the nodetree in general.

danielhollas · 2023-10-09T12:43:04Z

Just to clarify, I did not mean to implement it directly in the Node tree widget, just as a simple standalone button somewhere next to it. You also highlight an important point, having just an icon without any explanation is very cryptic, whereas an big red scary "Kill workflow" button would be more obvious.

mikibonacci · 2023-10-09T13:57:36Z

Dear @danielhollas thank you for your feedback! indeed, I think that the button should be somewhere near the workchain tree. Maybe for this release we can still have the button near the other two (new and refresh), and then in the future put it in a more reasonable place?

unkcpz

One more minor thing to change, others are all good to me.

unkcpz · 2023-10-10T09:03:54Z

src/aiidalab_qe/common/process.py

+        ipw.dlink(
+            (self, "value"),
+            (self.kill_work_chains_button, "disabled"),
+            lambda workchain: orm.load_node(workchain).is_terminated
+            if isinstance(workchain, int)
+            else True,
+        )


Instead of using dlink, I think it is more proper to set the disable of the button in the callback function _observe_value. Because:

the dlink is a bit complex with the transform from uuid to the running status, which will be clear if it is a regular code block in the function.

In the refresh_work_chain method, the disable of the button is check and set, this has the same logic as dlink here and the refresh should trigger the callback (it is already the case I assume, if not we need to fix it).

moving the disabling of the kill button into the _observe_value method.
However, in the init method I had to add the disabling, otherwise if
the browser page is refreshed (or loaded for the first time), it will not be
automatically disabled. I still do not understand why, but it works and the logic is not too involved.
@unkcpz please let me know if there is a smarter way to do it!

unkcpz · 2023-10-10T09:12:13Z

Just to clarify, I did not mean to implement it directly in the Node tree widget, just as a simple standalone button somewhere next to it.

I see, sorry for misunderstanding it. I guess you mean add it here,

aiidalab-qe/src/aiidalab_qe/app/result/__init__.py

Lines 25 to 30 in 967c3f8

    
           self.node_view = AiidaNodeViewWidget(layout={"width": "auto", "height": "auto"}) 
        
           ipw.dlink( 
        
               (self.process_tree, "selected_nodes"), 
        
               (self.node_view, "node"), 
        
               transform=lambda nodes: nodes[0] if nodes else None,

I can see the advantage of putting the button here, but I also thing having the kill button with "new", "refresh" is compatible with its usage. Let's discuss this in the meeting and see what others think.

unkcpz · 2023-10-16T09:23:01Z

Hi @mikibonacci, as we discussed during the meeting, we agree on that this button better to be placed above the node tree. Can you move it to there, thanks.

mikibonacci · 2023-10-17T14:40:38Z

Kill button now is in the step 4, just above the NodeTree. Also, if no workchain is shown, the button is not shown either. And is enabled only if the workchain is running/waiting/created.

@unkcpz @danielhollas

(still no effect on click button)

it uses the control module from aiida.engine.processes

for more information, see https://pre-commit.ci

However, in the init method I had to add the disabling, otherwise if the browser page is refreshed (or loaded for the first time), it will not be automatically disabled. I still do not understand why

for more information, see https://pre-commit.ci

if no workchain is selected, the button is not shown.

for more information, see https://pre-commit.ci

unkcpz

We had an offline pair coding and tidying up a bit on the implementation. All good from me. Thanks a lot! @mikibonacci

danielhollas · 2023-10-19T16:43:57Z

Nice, can you post a screenshot of the final product? When thinking about this, I couldn't figure out where is the best place to put it. Dne čt 19. 10. 2023 17:29 uživatel Jusong Yu ***@***.***> napsal:

…

Merged #500 <#500> into main. — Reply to this email directly, view it on GitHub <#500 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIY64OED5IBHMYLAIWUHLLYAFIN3AVCNFSM6AAAAAA5YR6TOOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQG4YTINJQGE4TOOI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

unkcpz · 2023-10-20T10:13:58Z

Here it is:

We just made the component in but didn't spend time on the layout.

mikibonacci requested a review from unkcpz October 9, 2023 10:20

unkcpz requested changes Oct 9, 2023

View reviewed changes

src/aiidalab_qe/common/process.py Outdated Show resolved Hide resolved

src/aiidalab_qe/common/process.py Outdated Show resolved Hide resolved

unkcpz requested changes Oct 10, 2023

View reviewed changes

mikibonacci changed the title ~~Feature/kill button~~ Feature/kill button - adding the kill button on top of NodeTree Oct 12, 2023

unkcpz closed this Oct 16, 2023

unkcpz reopened this Oct 16, 2023

mikibonacci and others added 11 commits October 19, 2023 16:24

"kill" button added in the workchain selector

a9f98b4

(still no effect on click button)

"_on_click_kill_work_chain" function, to kill a running workchain.

0e076bf

it uses the control module from aiida.engine.processes

disabling kill button if already terminated wchain

f59f9fc

[pre-commit.ci] auto fixes from pre-commit.com hooks

452042c

for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks

869e5e8

for more information, see https://pre-commit.ci

moving the disabling of the kill button into the _observe_value method.

e36e7ef

However, in the init method I had to add the disabling, otherwise if the browser page is refreshed (or loaded for the first time), it will not be automatically disabled. I still do not understand why

[pre-commit.ci] auto fixes from pre-commit.com hooks

63b9586

for more information, see https://pre-commit.ci

kill button in step 4, just above the NodeTree.

ac1a969

if no workchain is selected, the button is not shown.

[pre-commit.ci] auto fixes from pre-commit.com hooks

cf5b00d

for more information, see https://pre-commit.ci

Just removing unnecessary import

023f077

Move the kill button layout change to a dedicate function

df1d435

unkcpz force-pushed the feature/kill_button branch from e1b42a3 to df1d435 Compare October 19, 2023 16:24

unkcpz approved these changes Oct 19, 2023

View reviewed changes

unkcpz merged commit c8d1283 into main Oct 19, 2023
10 checks passed

unkcpz deleted the feature/kill_button branch October 19, 2023 16:29

danielhollas mentioned this pull request Mar 5, 2024

Add kill workflow button ispg-group/aiidalab-ispg#234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/kill button - adding the kill button on top of NodeTree #500

Feature/kill button - adding the kill button on top of NodeTree #500

mikibonacci commented Oct 9, 2023 •

edited by unkcpz

Loading

unkcpz left a comment

unkcpz commented Oct 9, 2023

codecov bot commented Oct 9, 2023 •

edited

Loading

danielhollas commented Oct 9, 2023

unkcpz commented Oct 9, 2023

danielhollas commented Oct 9, 2023

mikibonacci commented Oct 9, 2023

unkcpz left a comment

unkcpz Oct 10, 2023

mikibonacci Oct 10, 2023 •

edited

Loading

unkcpz commented Oct 10, 2023

unkcpz commented Oct 16, 2023

mikibonacci commented Oct 17, 2023

unkcpz left a comment

danielhollas commented Oct 19, 2023 via email

unkcpz commented Oct 20, 2023

Feature/kill button - adding the kill button on top of NodeTree #500

Feature/kill button - adding the kill button on top of NodeTree #500

Conversation

mikibonacci commented Oct 9, 2023 • edited by unkcpz Loading

unkcpz left a comment

Choose a reason for hiding this comment

unkcpz commented Oct 9, 2023

codecov bot commented Oct 9, 2023 • edited Loading

Codecov Report

danielhollas commented Oct 9, 2023

unkcpz commented Oct 9, 2023

danielhollas commented Oct 9, 2023

mikibonacci commented Oct 9, 2023

unkcpz left a comment

Choose a reason for hiding this comment

unkcpz Oct 10, 2023

Choose a reason for hiding this comment

mikibonacci Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

unkcpz commented Oct 10, 2023

unkcpz commented Oct 16, 2023

mikibonacci commented Oct 17, 2023

unkcpz left a comment

Choose a reason for hiding this comment

danielhollas commented Oct 19, 2023 via email

unkcpz commented Oct 20, 2023

mikibonacci commented Oct 9, 2023 •

edited by unkcpz

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

mikibonacci Oct 10, 2023 •

edited

Loading